Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Ansatz to be a concrete type with lattice information #223

Merged
merged 64 commits into from
Nov 4, 2024

Conversation

mofeing
Copy link
Member

@mofeing mofeing commented Oct 26, 2024

As discussed offline, we decided to slice #204 into smaller PRs. This first PR makes Ansatz a concrete type with lattice information contained in a graph.

The important thing you should review is the API, not the details; we can change the graph type in the future but we want the API to be stable.

Changelog

  • Introduced Lattice graph type
    • It's basically a Graphs.SimpleGraph with a bijective mapping between vertices and Sites
    • It adds Graphs and MetaGraphsNext as dependenciesy
  • Make Ansatz a concrete type in the type hierarchy
    • Its abstract counterpart is AbstractAnsatz
  • Delete Chain, Product, Grid and Dense
    • These will be re-added in subsequent PRs, adapted to the new Ansatz layer
  • Make Form the canonical form information trait
  • Refactor some evolve! methods based on simple-update to simple_update! and generalize it for any AbstractAnsatz
  • Added @testset_skip macro to implement @jofrevalles's idea of skipping whole testsets temporarily

Notes

  • Currently there is a bug in Julia makes Enzyme crash on an Ansatz due to its field lattice being abstract active_reg_inner crashes on Tuple{X} where {X} EnzymeAD/Enzyme.jl#1923 Should no longer be necessary because we replaced MetaGraph for Lattice
    • It's fixed in Julia 1.11.0 and should be fixed in Julia 1.10.6 1.10.7
    • I temporarily fixed it by fixing Ansatz to just 1D connectivity (i.e. Site{1}) but I'm removing it now -> For the time being you will need Julia 1.11 if you want to diff an Ansatz
  • There are other bug fixes (like Base.in or GraphMakie.graphplot! on AbstractTensorNetwork) that I'm gonna remove now and move to other PRs Already pushed to master
  • Might change Lattice to Topology in the future, because it's more accurate if we're not using the partial ordering property of lattices
  • In the future, we might change the subjacent graph type in Lattice for custom ones to accommodate clustered sites (i.e. a tensor containing 2+ sites) or hypergraphs

@mofeing mofeing added refactor Change internals triage Needs group consensus breaking-change labels Oct 26, 2024
@mofeing mofeing requested review from a team, arturgs, jofrevalles and Todorbsc October 26, 2024 22:14
src/Tenet.jl Show resolved Hide resolved
@jofrevalles
Copy link
Member

@mofeing @starsfordummies I think that when we talked about splitting the large PR into small ones, we meant that this PR would only contain the new Ansatz struct, and I see here that you pushed the commits regarding the canonical form, which IMO they should be in another PR. In this way, we can refactor the tests that are failing (like Chain, Dense, ...) in another PR.

@mofeing
Copy link
Member Author

mofeing commented Oct 29, 2024

@mofeing @starsfordummies I think that when we talked about splitting the large PR into small ones, we meant that this PR would only contain the new Ansatz struct, and I see here that you pushed the commits regarding the canonical form, which IMO they should be in another PR. In this way, we can refactor the tests that are failing (like Chain, Dense, ...) in another PR.

Sorry, but I don't agree. The Form trait is part of Ansatz, even if it doesn't have a field for that.

The other ansatzes will be reimplemented in subsequent PRs because tests had to be changed.

@mofeing mofeing force-pushed the concretize-ansatz branch 2 times, most recently from 7a870b7 to 0bef4a9 Compare October 29, 2024 09:36
Copy link
Member

@jofrevalles jofrevalles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have @test_skip instead of commenting all the tests.

test/integration/YaoBlocks_test.jl Outdated Show resolved Hide resolved
test/Product_test.jl Outdated Show resolved Hide resolved
test/Ansatz_test.jl Show resolved Hide resolved
src/Ansatz/Ansatz.jl Outdated Show resolved Hide resolved
@mofeing
Copy link
Member Author

mofeing commented Oct 29, 2024

I prefer to have @test_skip instead of commenting all the tests.

That's a valid request and a good idea. Will change it now and recover the deleted tests as @test_skip... The only thing is I'm not sure if it will still try to evaluate the code, which will obviously crash because we're breaking the API. So maybe @test_broken but doesn't matter which.

@mofeing mofeing requested a review from jofrevalles November 3, 2024 01:34
test/runtests.jl Outdated Show resolved Hide resolved
src/Ansatz.jl Show resolved Hide resolved
Copy link
Contributor

@Todorbsc Todorbsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jofrevalles
Copy link
Member

If we fix the documentation I guess we can merge

@mofeing
Copy link
Member Author

mofeing commented Nov 4, 2024

If we fix the documentation I guess we can merge

Docs are basically broken. We need to redo them but after this PR.

@jofrevalles jofrevalles self-requested a review November 4, 2024 14:23
Copy link
Contributor

@starsfordummies starsfordummies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sergio breaks tenet in all possible ways without even documenting what's broken. Typical commit, looks good to me 👍

@mofeing mofeing merged commit a432fa3 into master Nov 4, 2024
4 of 5 checks passed
@mofeing mofeing deleted the concretize-ansatz branch November 4, 2024 15:22
mofeing added a commit that referenced this pull request Nov 7, 2024
* Refactor `Product` on top of new `Ansatz` type

* Format code

* Fix typo

* Refactor `adapt_structure` method to support additional types

* Refactor `Reactant.make_tracer`, `Reactant.create_result` methods on top of recent changes

* Refactor `ChainRules` methods on top of new types

* Aesthetic name fix

* Stop using `IdDict` on Reactant extension

* Refactor lattice generation in constructors of `Dense`, `Product`, `MPS`, `MPO`, `PEPS`

* Reenable `Product` tests

* fix constructors

* remove `zeros`, `ones` (not well defined)

* fix symbol import

* export `Product`

* fix constructors

* refactor tests

* fix typo

* move file

* Document `Product` constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change refactor Change internals triage Needs group consensus
Projects
None yet
5 participants